Improve SentryTraceHeader constructor parameter validation #4604
Conversation
alexander-alderman-webb
requested review from
adinauer,
romtsn,
stefanosiano,
markushi and
lcian
as code owners
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d3af44e | 443.38 ms | 468.08 ms | 24.70 ms |
| 75111c8 | 414.61 ms | 448.59 ms | 33.98 ms |
| 38c63b5 | 414.92 ms | 431.77 ms | 16.85 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d3af44e | 1.58 MiB | 2.09 MiB | 521.84 KiB |
| 75111c8 | 1.58 MiB | 2.09 MiB | 521.57 KiB |
| 38c63b5 | 1.58 MiB | 2.09 MiB | 521.57 KiB |
There was a problem hiding this comment.
Overall looks good to me, please see the comments.
I've noticed that in the constructor of SentryId there's a case where we would throw a more specific exception about the format of the UUID, but it also mentions that UUIDs could be formatted with dashes which is not true in this particular case as the spec states that the trace id should be exactly 32 hex characters, so I think this approach is fine.
There was a problem hiding this comment.
LGTM!
While there's handling code in the SentryId ctor, it wouldn't apply here previously anyways, since the split on dashes wouldn't allow 36 char trace IDs that include dashes anyways and the check for corrupted IDs also wouldn't work since it looks for dashes as well (#2363).
I'm not aware of any deviations from span ID format, so more rules there should not be a problem.
Only accepting 0/1 for sampled should also not be a big deal.
Thanks for taking care of this!
alexander-alderman-webb
deleted the
feat/improve-sentry-trace-header-validation
branch
📜 Description
Use similar, regex-based, validation logic to the PHP SDK for the sentry-trace HTTP header: https://github.com/getsentry/sentry-php/blob/master/src/Tracing/TransactionContext.php#L9
#skip-changelog
💡 Motivation and Context
💚 How did you test it?
Added test cases with malformed arguments, which would have caused errors later on previously. These include tests with IDs that are too short or too long, and IDs with invalid characters.
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps